Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: AVRCP: connection and disconnection of AVRCP #75637

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

gzh-terry
Copy link

@gzh-terry gzh-terry commented Jul 9, 2024

This pull request implemented the basic AVRCP functions, which includes:

  1. Establishing AVCTP channel.
  2. Create AVRCP instance.
  3. Register SDP services.
  4. A shell function to test these interfaces.

This is the first part of the entire AVRCP functions, we tested the code by
establishing AVCTP channel with Android phones.

Four configs are provided to turn on/off the functions:

  1. CONFIG_BT_AVRCP_TARGET
  2. CONFIG_BT_AVRCP_CONTROLLER
  3. CONFIG_BT_AVRCP
  4. CONFIT_BT_AVCTP

@gzh-terry
Copy link
Author

Hi @lylezhu2012 ,

May I doublecheck if this is the correct method to start a new feature?
AVRCP would include a series of pull requests. Will it be better if I submit them together?

Best regards,
Zihao

@carlescufi
Copy link
Member

May I doublecheck if this is the correct method to start a new feature? AVRCP would include a series of pull requests. Will it be better if I submit them together?

@gzh-terry thanks for your PR. Yes, this is fine, you can submit a part of the implementation in a first Pull Request and then later send follow-up PRs with additional functionality.

subsys/bluetooth/host/classic/avctp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avctp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avctp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avctp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avctp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved
Comment on lines +2079 to +2091
if (IS_ENABLED(CONFIG_BT_AVCTP)) {
bt_avctp_init();
}

bt_sdp_init();

if (IS_ENABLED(CONFIG_BT_A2DP)) {
bt_a2dp_init();
}

if (IS_ENABLED(CONFIG_BT_AVRCP)) {
bt_avrcp_init();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't particularly scalable. It should be possible to implement new profiles/protocols completely on the application side, which means that you can't have a requirement that the stack-internal l2cap_br.c needs to be modified every time you add some new profile. The profiles/protocols which are part of the Bluetooth core specification (L2CAP, SDP, etc) could be treated as an exception, but for everything else you need to find a solution which doesn't require modifying this internal c-file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure why A2DP and AVDTP is placed here.
Nevertheless, we should put them together rather than write AVRCP elsewhere.

Zihao Gao added 8 commits October 15, 2024 20:47
This patch implementing avrcp.c
New Kconfig BT_AVRCP is provided to enable this layer.
BT_AVRCP_TARGET and BT_AVRCP_CONTROLLER are then
provided to enable one of the two roles independently.
avrcp.h shows the APIs for the upper layer.

Only connection and disconnection interfaces are provided in this patch.

Signed-off-by: Zihao Gao <[email protected]>
This patch add SDP records for both CT and TG role.

The SDP attribute would be registered according to the configuration.

OBEX and Browsing commands are optional and yet not supported.

SDP registration is implemented at AVRCP level to simplify the
workload of the upper layer. We assume the App can have limited
knowledge on SDP structures.

Signed-off-by: Zihao Gao <[email protected]>
Only the basic functions for establishing an AVCTP connection
are provided at this stage.
An BR/EDR ACL connection is necessary before AVRCP function.
Register callbacks before utilizing AVRCP.

Signed-off-by: Zihao Gao <[email protected]>
This patch defines the message format for general AVCTP.
They would be further called by AVRCP layer.

Signed-off-by: Zihao Gao <[email protected]>
This patch defines the message format for AVCTP unit message.
This is the first out of the four types of commands and can be
used by the CT to obtain the unit info from the TG device.

Signed-off-by: Zihao Gao <[email protected]>
This patch allow to acquire the unit info of the remote device.

Signed-off-by: Zihao Gao <[email protected]>
This patch received an AVCTP message and forward to the
upper layer, e.g., AVRCP.

Signed-off-by: Zihao Gao <[email protected]>
This patch received an AVRCP message and remove timeout timers.

Signed-off-by: Zihao Gao <[email protected]>
jhedberg
jhedberg previously approved these changes Oct 15, 2024
@gzh-terry
Copy link
Author

Hi @jhedberg,

Many appreciates for your quick review.
Still some warnings to fix. would re-request review later.

The bit order can be incorrect when use bit field definition.

Signed-off-by: Zihao Gao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants